(from the PR description)

In case of containers there is no reliable way to know how the command execution went, and the most important here is to fetch logs, so I've built API which returns an IAsyncEnumerable - basically a stream of logs.

Not sure what you mean. The status for ContainerExec object has a State property and an ExitCode property https://github.com/dotnet/aspire/blob/main/src/Aspire.Hosting/Dcp/Model/ContainerExec.cs#L56 so you can know extactly what the outcome of an container executable was, once it has completed.

OK, so I've reviewed this code. I definitely do not want us to introduce ResourceCommandAnnotationBase. I see this as unnecessary. I think the reason it feels necessary is that we've got an inverted relationship between ResourceContainerExecCommandAnnotation and ContainerExecService.

What we should be doing is in the WithExecCommand(...) method we should be invoking WithCommand(...) and into the callback we should be making a call to the DI container to resolve the ContainerExecService (or its interface) and then using that to invoke the command in the selected container resources. The parameters that are used to invoke the API on ContainerExecService would be passed in to the WithExecCommand(...) and captured in the lambda that is passed into the WithCommand(...) extension method.

We should look to WithHttpCommand(...) for inspiration here. the WithExecCommand(...) should probably have the essential arguments that we need for invoking a command inside the container - plus a derived ExecCommandOptions type.

One thing that we should probably think carefully about here is whether it actually makes sense to define container exec commands in the app model as a new kind of resource. Presently we have ContainerResources - perhaps we should have a ContainerExecutableResource which derives from ExecutableResource.

This way we might be able to take advantage of all of the plumbing we already have in Aspire around handling log streaming etc.

I believe that aspire exec will need to be converted with aspire run and that when doing aspire exec we will need to show the dashboard. We should probably resolve this before going too much further.

@DeagleGross please look to address some of the feedback I've added as comments to this commit in this PR:

#10380 (comment)_

#10380 (comment)_

If we are introducing a resource into the app model and we implement interfaces like IResourceWithArgs we need to make sure that we honor the semantics of those interfaces. That means things like the Args property on the ContainerExecutableResource is redundant because we capture arguments in a series of callback arguments which get set with things like WithArgs(...).

Additionally instead of having a TargetResource property we should just implement IResourceWithParent I think.

@mitchdenny thanks for the explanation about similarities with WithCommand, but I disagree - the API is simply different. In case of container exec underlying execution happens in a separate DCP resource, and we should have all the infra working for us providing a way to fetch logs for example. Trying to pack everything into WithCommand with its hardly binded Func<ExecuteCommandContext, Task<ExecuteCommandResult>> contract is not extensible enough. Please see the updated code and the idea by Karol here: #10779 (comment).

However, I've refactored the code and removed the base annotation class etc, so its cleaner now.

The cli changes for aspire exec to be based on top of aspire run is unrelated to this flow, and should come in a separate PR. I will also definitely fix what you have described in other PR (#10380 (comment)_
#10380 (comment)_); but will do in a separate PR for simplicity, if you are fine with that - this PR should not make it harder, because it provides a very simple command definition and args are not touched here

@mitchdenny thanks for the explanation about similarities with WithCommand, but I disagree

I think there might be a bit of a disconnect here. When I look at WithExecCommand(...) I initially thought of it as being analogous to WithHttpCommand(...). But in reality, WithExecCommand(...) is much more like a child resource of a ContainerResource.

With that in mind I'm wondering if we are conflating two things here - commanding on the dashboard, and the introduction of a sub-resource for containers capable of executing resources within a running container.

If we imagine that dashboard commands don't exist for a moment, the way we might add this feature is that we would do something like this:

var builder = DistributedApplication.CreateBuilder(args);
builder.AddContainer("mycontainer", "myimage")
       .AddInvocation("truncate-database", "dbtool", "truncate");

Note I'm using AddInvocation(...) here as a stand-in for *Command* or *Exec* because I think that the usage is a bit overloaded.

What the code above would do is that the container would start, and there would be an implicit WaitFor(...) on the parent resource before the truncate-database resource executed.

Now - the interesting here is how this maps onto aspire exec. For aspire exec you wouldn't have the call to AddInvocation(...) instead you would probably just use --resource to identify the parent container resource - and if it is a container resource instead of adding a ExecutableResource add a ContainerExecutableResource.

This doesn't really have anything to do with dashboard commands - in fact they aren't even really required. Because ContainerExecutableResource is a resource and has lifecycle and logs and all that good stuff - it would be visble in the dashboard. If you wanted to define a ContainerExecutableResource using something like AddInvocation(...) then you could but you might also append WithExplicitStart(...) to stop it starting automatically - and when you wanted to run it you would just click start in the dashboard.

You could - if you wanted to also hide the resource so that it is not alway visible and then add a command using WithCommand(...) (assuming you are building an integration that is layered on top of all this).

That WithCommand(...) would use the ResourceCommandService to simply start the container executable resource just like any other resource and potentially make the resource visible in the dashboard and present an interaction dialog which jumps the user to the logs.

@mitchdenny and @DeagleGross, just wanted to point out that ContainerExec in DCP represents a single invocation of a command within a container. It is not a "template" for command execution. ContainerExec objects can be created at any time during the corresponding container lifetime.

I guess how we incorporate this capability into Aspire model is up for debate. We need to consider deployment environments which may or may not have that capability for containers. Personally, I am not very sure if modeling "execution of programs within containers" (avoiding the overloaded term "command" here) is a good candidate for being a distinct resource type in Aspire model. I am wondering if something more lightweight (e.g. just a method on the container resource) would ultimately be easier to use and more appropriate.

Personally, I am not very sure if modeling "execution of programs within containers" (avoiding the overloaded term "command" here) is a good candidate for being a distinct resource type in Aspire model. I am wondering if something more lightweight (e.g. just a method on the container resource) would ultimately be easier to use and more appropriate.

This is exactly my thinking too. I do not see this as a resource at all, but rather exposing the ability to execute/invoke something in the context of a resource.

My concern is that if we don't model this as a resource from top to bottom then we are going to end up with unnecessary duplications for something that conceptually is very well aligned with resources in the Aspire app model.

I mean we define models in Ollama as resources - why not a docker exec invocation? We've found that even for resources that aren't executables that there is a need to be able to inspect the output that they produce - it just makes sense that we would use the existing dashboard infrastructure for this doesn't it?

This submission has been automatically marked as stale because it has been marked as requiring author action but has not had any activity for 14 days.
It will be closed if no further activity occurs within 7 days of this comment.